Extend billing service #567
Conversation
|
|
|
|
|
|
|
zugao
force-pushed
the
extended-billing
branch
from
de1b436 to
b3b7cbe
Compare
|
|
zugao
force-pushed
the
extended-billing
branch
from
37a5950 to
bee2705
Compare
|
zugao
force-pushed
the
extended-billing
branch
from
f6b427e to
d952654
Compare
|
|
|
zugao
requested review from
a team,
Kidswiss,
TheBigLee,
mdnix and
mikeshootzz
and removed request for
a team
|
zugao
force-pushed
the
extended-billing
branch
from
8e78325 to
173eeea
Compare
|
zugao
force-pushed
the
extended-billing
branch
from
173eeea to
d318fa5
Compare
|
zugao
force-pushed
the
extended-billing
branch
from
d318fa5 to
ebf8832
Compare
|
|
zugao
force-pushed
the
extended-billing
branch
from
5c3ef51 to
41436b9
Compare
|
Kidswiss
left a comment
Kidswiss
left a comment
There was a problem hiding this comment.
Some small things, the rest LGTM
|
|
||
| // If no items specified, create default compute item | ||
| items := opts.Items | ||
| if len(items) == 0 { |
There was a problem hiding this comment.
Shouldn't we always inject the service?
If we have a service with multiple items we'd have to pass the service itself explicitly as well, which could lead to a lot of dupes, since most servala services will have more than one item anyway.
There was a problem hiding this comment.
should we make this mandatory and configure as mandatory via kubebuilder?
| itemDescription := GetItemDescription(isAPPUiOCloud, clusterName, namespace) | ||
|
|
||
| // If no items specified, create default compute item | ||
| items := opts.Items |
There was a problem hiding this comment.
In accordance with my comment bellow, this field should then be called ExtraItems istead. To make it clear we additional items to the default service item.
There was a problem hiding this comment.
no need for extra items. Whoever is creating a BIllingService CR should add at least one item in the list.
|
|
||
| // Find oldest sent events for this product | ||
| // Only prune events with status "sent" | ||
| prunableIndices := []int{} |
There was a problem hiding this comment.
How can we get rid of entries that failed?
We might need some configuration to specify how many failed events we should keep.
There was a problem hiding this comment.
we should keep all of them but to make sure we don't create duplicates. If we remove the ones that were not sent we loose history and we cannot ensure that Odoo has correct data. I would be very cautious with removing events that are not yet in odoo.
mikeshootzz
left a comment
mikeshootzz
left a comment
There was a problem hiding this comment.
Agree with @Kidswiss but the rest looks good to me. One small thing:
| - xobjectbuckets | ||
| verbs: | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch | ||
| - apiGroups: | ||
| - apiextensions.crossplane.io | ||
| resources: | ||
| - compositeresourcedefinitions | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
Don't forget to rebase to develop and run make generate to fix this
Summary
Checklist
/mergecomment.Component PR: vshn/component-appcat#1037